Support card reader auto-reconnection#16586
Conversation
- Add CardReaderReconnectionState enum with idle, reconnecting, succeeded, and failed states - Add reconnectionEvents publisher and cancelReconnection() to CardReaderService protocol - Add reconnectionCancellation error case to CardReaderServiceError - Implement reconnection delegate methods in StripeCardReaderService - Add no-op implementations to NoOpCardReaderService
- Add observeCardReaderReconnectionState action to publish reconnection state changes - Add cancelReconnection action to cancel in-progress auto-reconnection - Add CardReaderReconnectionState typealias to Model.swift - Implement action handlers in CardPresentPaymentStore
- Add reconnecting case to CardPresentPaymentReaderConnectionStatus - Add cancelReconnection() to CardPresentPaymentFacade protocol - Implement no-op cancelReconnection in CardPresentPaymentPreviewService - Handle reconnectionCancellation error in CardPresentPaymentsRetryApproach
- Subscribe to reconnection state publisher and map to connection status - Implement cancelReconnection() to dispatch cancel action - Cancel any ongoing reconnection before starting manual connection
- Add reconnecting case to CardReaderConnectionStatusView with spinner and cancel menu - Handle reconnecting case in TotalsView payment UI logic - Add cancelReconnection() method to PointOfSaleAggregateModel
- Add reconnection simulation methods to MockCardReaderService - Add tests for observeCardReaderReconnectionState action - Add test for cancelReconnection action - Update MockCardPresentPaymentService with cancelReconnection - Add cancelReconnection test to PointOfSaleAggregateModelTests - Update CardPresentPaymentServiceScreenshotMock
Generated by 🚫 Danger |
staskus
changed the title
staskus
added
the
feature: mobile payments
|
|
- Observe reconnection state in BluetoothCardReaderSettingsConnectedViewModel - Keep showing connected reader view during reconnection - Disable update/disconnect buttons and show spinner during reconnection
Cancel any ongoing auto-reconnection before initiating a new Bluetooth reader discovery to prevent conflicts.
Add cancelReconnection action handling to MockCardPresentPaymentsStoresManager and RefundSubmissionUseCaseTests to prevent test timeouts.
Update MainTabBarControllerTests to handle the .observeCardReaderReconnectionState action in the test action handler. Otherwise, initialization of CardPresentPaymentService stalls the test.
Configure the Stripe Terminal SDK to automatically attempt reconnection when a Bluetooth reader unexpectedly disconnects.
Update the didStartReconnect delegate method to use the new Stripe SDK signature that includes the disconnect reason parameter. Also clear connected readers when reconnection starts so the UI correctly shows the reconnecting state instead of connected.
When canceling reconnection, if the Stripe SDK returns error code cancelFailedAlreadyCompleted (1010), treat it as success rather than an error. This race condition occurs when reconnection completes naturally just before the cancel request is processed. Since the user's intent to stop reconnection was effectively achieved, there's no need to log an error.
Stripe: Adjust reconnection cancellation handler to avoid clearing connectedReaders when cancellation failed due to already-completed reconnection; only clear readers when cancellation actually succeeded or failed for other reasons, and ensure reconnectionState is set to .idle in the appropriate branches. Yosemite store: Replace .subscribe(Subscribers.Sink(...)) with .sink(...) and store the returned AnyCancellable in the cancellables set to retain the subscription. WooCommerce adaptor: Remove the preemptive await cancelReconnection() call from connectReader to avoid unnecessarily cancelling/interrupting concurrent reconnection logic before starting a manual connection.
- Show "Reconnecting to card reader..." status in connected view - Add "Cancel Reconnection" button during reconnection - Preserve reader info (name, battery, firmware) during reconnection - Prevent searching view from showing during reconnection - Refactor button states using enums for cleaner code
- Show reader info during reconnection via PointOfSaleSettingsController - Add reconnecting menu button with cancel option - Disable firmware update during reconnection - Add test for reconnecting state providing reader info - Update mock to support connection status directly
Show "Reconnecting reader..." message when reader is reconnecting during checkout. This prevents the confusing "Scanning for readers" popup by not auto-starting payment collection during reconnection.
A no-op cancel should succeed rather than fail, consistent with how the real service treats cancellation when no reconnection is in progress.
All call sites provide a non-nil value, so the optional adds no safety and silently no-ops if accidentally nil. Matches connectCardReaderAction.
Only log the error when a reconnection was actually in progress to avoid noisy log output from expected SDK callbacks.
|
@joshheald @iamgabrielma I integrated the new pos payment model changes, retested everything, and made a few fixes. |
|
Version |
|
Guessing you want another review on this one, right? I'll take a look tomorrow if so 😊 |
|
@joshheald ah yes, thanks for reminding! |
|
@staskus Would you mind wrangling the conflicts? Sorry, I took a look but not enough context to do it quickly |
Refactor Stripe card reader reconnection flow to safely handle a nil self early and clean up the reconnection cancelable state. Suppress noisy error logs when a reconnection failure occurs after the user has cancelled, and add an error log if cancelling reconnection via CardPresentPaymentAction fails. Also fix an incorrect import path for CardReaderSoftwareUpdateState.
|
Sorry, promise I'll get to this one tomorrow, travel time today took longer than I thought. |
joshheald
left a comment
joshheald
left a comment
There was a problem hiding this comment.
Works well, and a good improvement to the app. Various question/suggestions in line.
Behaviour – the only thing I noticed was that the payment method buttons are unresponsive for a few seconds during a reconnection, while we wait for the reconnection cancellation to complete. A good follow up might be to show a card payment screen with a spinner as soon as a payment method is tapped, if we're cancelling a reconnection before we can start the new one. Not sure how much complexity that would add though... and it should be quite a rare case.
| HStack(spacing: Constants.buttonImageAndTextSpacing) { | ||
| ProgressView() | ||
| .progressViewStyle(POSProgressViewStyle( | ||
| size: Constants.progressIndicatorDimension * scale, | ||
| lineWidth: Constants.progressIndicatorLineWidth * scale | ||
| )) | ||
| Text(isCancellingReconnection ? Localization.cancellingReconnection : Localization.readerReconnecting) | ||
| .foregroundColor(connectedFontColor) | ||
| } | ||
| .padding(.horizontal, Constants.horizontalPadding) | ||
| .frame(maxHeight: .infinity) | ||
| } | ||
| .disabled(isCancellingReconnection) | ||
| .accessibilityIdentifier("pos-reader-reconnecting") |
There was a problem hiding this comment.
Can we use progressIndicatingCardReaderStatus here too? Even if it needs a little extending?
| "pointOfSale.floatingButtons.disconnectCardReader.button.title", | ||
| value: "Disconnect Reader", | ||
| "pointOfSale.floatingButtons.disconnectCardReader.button.title.2", | ||
| value: "Disconnect reader", |
There was a problem hiding this comment.
I'm not sure how closely we're still following pecCkj-eD-p2 any more, but I still find it strange to see sentence case on buttons on iOS.
There was a problem hiding this comment.
I'm not sure how closely we're still following pecCkj-eD-p2 any more
I haven't heard about this P2 myself. It looks like in POS we do use sentence case in a few places: "Connect your reader", "Update firmware", "Cash payment", "Check out". It's a change that we can make in one swoop, but now it looks like the sentence case is the convention
| .padding(.horizontal, POSPadding.medium) | ||
| .background(Color.posSurfaceContainerLowest) | ||
| .cornerRadius(POSCornerRadiusStyle.small.value) | ||
| .overlay( | ||
| RoundedRectangle(cornerRadius: POSCornerRadiusStyle.small.value) | ||
| .strokeBorder(Color.posOnSurface, lineWidth: 2) | ||
| ) | ||
| } |
There was a problem hiding this comment.
could/should some of this be in a button style? No worries if it's too much of a faff.
There was a problem hiding this comment.
This is a Menu label rather than a Button, so ButtonStyle can't be applied directly. The inline spinner behavior also differs from the existing loading state in POSInfoCardButtonStyle (which replaces the label entirely). Since this is the only Menu with this styling, I'll leave it as-is for now.
| // When the order's being created or synced, we only show the shimmering totals. | ||
| // Before the order exists, we don't want to show the card payment status, as it will | ||
| // show for a second initially, then disappear the moment we start syncing the order. |
There was a problem hiding this comment.
Not sure that this comment really adds anything? Unless this is moved from elsewhere.
| switch paymentModel.cardReaderConnectionStatus { | ||
| case .connected, .disconnecting, .cancellingConnection: | ||
| switch displayPaymentState.activePaymentMethod { | ||
| case .cash: | ||
| return true | ||
| case .card: | ||
| return paymentModel.cardPresentPaymentInlineMessage != nil | ||
| } | ||
| case .reconnecting: | ||
| switch displayPaymentState.activePaymentMethod { | ||
| case .cash: | ||
| return true | ||
| case .card: | ||
| return paymentModel.cardPresentPaymentInlineMessage != nil || | ||
| totalsViewHelper.shouldShowReconnectingMessage(readerConnectionStatus: paymentModel.cardReaderConnectionStatus, | ||
| paymentState: displayPaymentState) | ||
| } | ||
| case .disconnected: | ||
| return true | ||
| } |
There was a problem hiding this comment.
This seems a lot of new code considering the change... in the view too. Can we simplify it? E.g. from a scan, it seems like the reconnecting case could be used for connected/disconnecting/cancellingConnection too? And/or the whole lot could be in the viewHelper for testing?
There was a problem hiding this comment.
You're right. It should be possible to simplify it.
| await withCheckedContinuation { continuation in | ||
| let action = CardPresentPaymentAction.cancelReconnection { result in | ||
| if case .failure(let error) = result { | ||
| DDLogError("⚠️ Failed to cancel reader reconnection: \(error)") | ||
| } | ||
| continuation.resume() | ||
| } | ||
| stores.dispatch(action) |
There was a problem hiding this comment.
From POS we use a nillableContinuation... do we need the same here?
There was a problem hiding this comment.
Good question. Well, it could be safer using nillableContinuation to avoid any double continuation calls.
| if !didGetConnectedReaders { | ||
| newShouldShow = .isUnknown | ||
| } else if connectedReaders.isEmpty { | ||
| } else if connectedReaders.isEmpty && !readerReconnectionInProgress && !readerReconnectionCancellationInProgress { |
There was a problem hiding this comment.
I think it's worth describing this compound logic in a variable. AND NOT is always one of the harder to read operators, at least for me, and we've got two in a row here.
| init(viewModel: BluetoothCardReaderSettingsConnectedViewModel) { | ||
| switch (viewModel.readerReconnectionInProgress, viewModel.optionalReaderUpdateAvailable) { | ||
| case (true, _): | ||
| self = .reconnecting | ||
| case (false, true): | ||
| self = .updateAvailable | ||
| case (false, false): | ||
| self = .upToDate | ||
| } |
There was a problem hiding this comment.
Were you able to test the update-on-reconnection case? I hadn't thought of it but I can't really see a great way to test it...
There was a problem hiding this comment.
The UpdatePromptState re-evaluates whenever the view model changes, so after reconnection completes (readerReconnectionInProgress flips to false), it should correctly show the update banner if optionalReaderUpdateAvailable is still true. Haven't been able to test this specific transition manually though.
| case .cancellingReconnection, .disconnecting, .updating: | ||
| return false | ||
| } |
There was a problem hiding this comment.
Some of this seems fragile. action silently does nothing in these cases, which is fine because the button's disabled... but perhaps the state could be structured so that we can't end up with different sets in these cases? The disconnect button used to have a pretty clear single action which would get called every time, so remaining closer to that would be good...
There was a problem hiding this comment.
I restructured it so the button state and action itself would be tied to one another.
|
@joshheald thanks for the review! I addressed most of the comments.
True. I looked at it, and it would be one more addition of complexity to the existing large PR. I agree that it will be a rarer case that is not as critical to handle right away. |
WOOMOB-2028
Description
Implements Stripe Terminal SDK auto-reconnection delegate methods for Bluetooth card readers. When a reader unexpectedly disconnects, the SDK automatically attempts to reconnect. Previously, the app did not handle these events, leaving the app state out-of-sync with the card reader connection.
Android PR: woocommerce/woocommerce-android#15047
The PR:
I didn't make any changes to the IPP payment flow. The way it's structured is that the payment would just fail if the card reader is disconnected. User can just dismiss it, turn on / reconnect the reader, and start a card reader flow again.
Tests
Tested on
Scenarios
I implemented explicit handling in these scenarios:
Do you think there are any other places I should explicitly handle reconnection state? Otherwise, it happens in the background and immediately reconnects if the card reader connects again.
Cases
In different scenarios, attempt to connect, and then disconnect (turn off) the reader.
Videos
POS Floating Button
POS.-.Floating.bar.MP4
POS Settings
ScreenRecording_01-30-2026.18-09-38_1.MP4
POS Payment
reconnection.in.payment.MP4
Payment -> Manage Card Reader
ScreenRecording_01-30-2026.17-37-54_1.MP4
POS Bookings
pos.bookings.reconnection.MP4
POS Payment Flow with disconnection and reconnection during the payment
ScreenRecording_03-10-2026.16-23-27_1.MP4
IPP Payment flow with disconnection and manual reconnection
It's not handled explicitly. The PR is large enough, but most importantly, the flow is not blocked.
IPP.Flow.mov